Skip to content

[CI] Remove the unused ECR cache lookup from pipeline generation - #433

Open
Hoiyeuhng wants to merge 1 commit into
vllm-project:mainfrom
Hoiyeuhng:fix/remove-unused-ecr-cache-resolution
Open

[CI] Remove the unused ECR cache lookup from pipeline generation#433
Hoiyeuhng wants to merge 1 commit into
vllm-project:mainfrom
Hoiyeuhng:fix/remove-unused-ecr-cache-resolution

Conversation

@Hoiyeuhng

@Hoiyeuhng Hoiyeuhng commented Jul 22, 2026

Copy link
Copy Markdown

Summary

  • stop injecting $CACHE_FROM and $CACHE_TO into step commands
  • remove get_ecr_cache_registry() and the two helpers only it used
  • add regression coverage that neither placeholder is injected

Net +9 / -97. Registry caching behavior is unchanged.

Why

_get_variables_to_inject() resolves the two placeholders through get_ecr_cache_registry(), which runs aws ecr get-login-password, docker login and up to two docker manifest inspect calls, then raises RuntimeError if the login fails.

Nothing consumes the result. In vLLM main's .buildkite/ tree the placeholders occur only inside image_build/image_build.sh, image_build/image_build_cpu.sh and scripts/ci-bake-rocm.sh, each of which computes its own values just before the build — true since vllm-project/vllm#29343 moved image builds under .buildkite/image_build/. Release branches v0.28.0, v0.27.0 and v0.9.2 have no occurrences either.

The other injected variables are still live, which is what isolates these two. image_build.yaml consumes five of the seven as positional arguments:

.buildkite/image_build/image_build.sh $REGISTRY $REPO $BUILDKITE_COMMIT $BRANCH $IMAGE_TAG $IMAGE_TAG_LATEST

So the cost today is less the round trip than that _get_variables_to_inject() reads as though the generator still owns cache-tag resolution.

Scope

bootstrap-intel.sh has its own bash resolve_ecr_cache_vars that also looks unused, but it sits on a live bootstrap path whose caller is configured in Buildkite rather than in this repo, so I left it alone. Happy to follow up if you confirm it.

Duplicate-work check

#394 changes ci.hcl and terraform rather than the generator, and #413 is explicitly foundation-only, so neither overlaps. #321 edits the same function, so whichever lands second needs a trivial conflict fix.

Validation

  • pytest buildkite/tests — 111 passed on Python 3.12 and on 3.9 (the pipeline agents run 3.9), installed the way the Pipeline generator tests workflow does.
  • The new test fails against current main before passing here.
  • pre-commit clean on the changed files except ruff-format, which reformats buildkite_step.py and test_step.py identically at main — pre-existing, so left out of this diff.

AI assistance

AI assistance was used to research, implement and test this change. The submitter has reviewed every changed line.

@Hoiyeuhng
Hoiyeuhng marked this pull request as draft July 22, 2026 20:57
@Hoiyeuhng Hoiyeuhng changed the title fix(buildkite): remove unused ECR cache lookup fix(buildkite): resolve ECR cache tags only when referenced Jul 22, 2026
@Hoiyeuhng
Hoiyeuhng force-pushed the fix/remove-unused-ecr-cache-resolution branch from a8ecf47 to c37b2af Compare July 22, 2026 21:29
@Hoiyeuhng
Hoiyeuhng marked this pull request as ready for review July 22, 2026 21:40
@Hoiyeuhng
Hoiyeuhng force-pushed the fix/remove-unused-ecr-cache-resolution branch 2 times, most recently from 6544601 to cd77146 Compare August 29, 2026 04:28
@Hoiyeuhng Hoiyeuhng changed the title fix(buildkite): resolve ECR cache tags only when referenced fix(buildkite): remove unused ECR cache lookup from pipeline generation Aug 29, 2026
@Hoiyeuhng
Hoiyeuhng force-pushed the fix/remove-unused-ecr-cache-resolution branch from cd77146 to fe577eb Compare August 29, 2026 04:55
@Hoiyeuhng Hoiyeuhng changed the title fix(buildkite): remove unused ECR cache lookup from pipeline generation [CI] Remove the unused ECR cache lookup from pipeline generation Aug 29, 2026
`_get_variables_to_inject` injected `$CACHE_FROM` and `$CACHE_TO` into
step commands, resolving them through `get_ecr_cache_registry()`. No step
command consumes either placeholder: vLLM's image builds moved into
`.buildkite/image_build/` (vllm-project/vllm#29343) and the scripts there
compute and export their own BuildKit cache variables.

The helper ran `aws ecr get-login-password`, `docker login` and up to two
`docker manifest inspect` calls on every pipeline generation, and raised
`RuntimeError` on authentication failure, so the dead path also carried a
failure mode. Registry caching itself is untouched.

Signed-off-by: Haiyang Luo <41023868+Hoiyeuhng@users.noreply.github.com>
@Hoiyeuhng
Hoiyeuhng force-pushed the fix/remove-unused-ecr-cache-resolution branch from fe577eb to 1593a75 Compare August 29, 2026 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant